Codecov Report

Attention: Patch coverage is 70.75472% with 31 lines in your changes missing coverage. Please review.

Project coverage is 41.72%. Comparing base (7c71c41) to head (9a10320).

@@                  Coverage Diff                  @@
##           feature/efm-recovery    #6744   +/-   ##
=====================================================
  Coverage                 41.71%   41.72%           
=====================================================
  Files                      2030     2031    +1     
  Lines                    180459   180552   +93     
=====================================================
+ Hits                      75285    75329   +44     
- Misses                    98978    99031   +53     
+ Partials                   6196     6192    -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

strategic / conceptual thoughts

1. I am thinking about enforcing activation of the protocol extension (specifically, the usage of the Chunk.ServiceEventIndices field). For example, it would be nice if consensus nodes could drop Execution Results with the deprecated format and avoid incorporating them into a block (and rejecting blocks that still do).

2. To ensure consensus incorporates only execution receipts following the new convention after a certain height, it would be great if we could include some consistency check also in the receiptValidator (somewhere around here).

3. I was wondering if your plan is still to remove Chunk.ServiceEventIndices field for Byzantine Fault Tolerance in the long term? I think we had talked about turning ExecutionResult.ServiceEventList into an indexed list. Or are you thinking about keeping the ServiceEventIndices field as the long-term solution -- just with removing the backwards-compatibility case?

   In general, my preference would be to also allow Chunk.ServiceEventIndices = nil when there are no service events generated in the chunk. Thereby the final solution becomes a lot more intuitive:

   • if ExecutionResult.ServiceEvents is not empty (nil allowed), then the new convention requires for consistency:

     $$\sum_\texttt{Chunk} \texttt{len}(Chunk.ServiceEventIndices) = \texttt{len}(\texttt{ExecutionResult.ServiceEventList})\qquad\qquad\qquad\qquad(1) $$

     I feel is check is very similar to other stuff, which consensus nodes already verify about an Execution Receipt (see ReceiptValidator implementation)

   • We would temporary relax the new convention, eq. $(1)$, for downwards compatibility as follows: the ServiceEventIndices fields of all chunks can be nil despite there being service events.

   • As service events are rare, ExecutionResult.ServiceEventList is empty in the majority cases. Then both the deprecated as well as the new conventions would allow ChunkBody.ServiceEventIndices to be nil (which is the most intuitive convention anyway). Also, for individual chunks that don't produce any service events, their ChunkBody.ServiceEventIndices could be nil or empty according to the new convention. Then, also the new convention is very self-consistent in my opinion - and the depreciation condition is only an add on that can be later removed.

I mean is this is only a temporary solution, I am happy and we can skip most of question 3.

Summary of Discussion with @AlexHentschel

Change of ServiceEventIndices structure

• Replaces list with ServiceEventsNumber, a uint16 which is a count of the number of service events emitted in that chunk
  • Since VNs have access to all chunks, they can easily compute the index range based on this field alone
  • This is a more compact, and fixed-size representation
  • Structural validation is simpler, we require only that sum(chunk.ServiceEventsNumber for chunk in chunks) == len(ServiceEvents)
• Backward compatibility:
  • If any service events are emitted in a result, and all ServiceEventsNumber fields are 0, then this is interpreted as a v0 model version: all service events must have been emitted in the system chunk.

Removing backward-compatibility at next spork

We plan to keep the overall structure as the long term solution, and only remove the backward-compatibility support at the next spork. We do not plan to use a different representation (ie. Chunk.ServiceEventIndices field).

Upgrade Comments

• Protocol HCUs are triggered by a ProtocolVersionUpgade service event. This service event is emitted outside the system chunk, meaning that the first Protocol HCU must take place after the service event validation fix has been deployed.
• We plan to incorporate all changes under feature/efm-recovery in one Protocol HCU.

Rough Outline of Process

1. Do a manual rolling upgrade of all node roles, to a version including feature/efm-recovery.
2. Manually verify that all ENs are upgraded (this is the only correctness-critical step!)
   • this is necessary prior to emitting the first ex-system-chunk service event
   • ideally, VNs are also updated, but we can rely on emergency sealing as a fallback if necessary
3. Emit ProtocolVersionUpgrade service event, scheduling the protocol upgrade at view V.
   • Nodes which are not upgraded when we enter view V will halt.
   • We must have a substantial majority (>> supermajority) of SNs and LNs for the network to remain live, before entering view V (this is the only liveness-critical step!)

Summary of discussion with @zhangchiqing

Let's call v0 the old version and v1 the new version. Leo pointed out the v0 nodes will fail to validate data models produced by v1 nodes. In particular, this step:

Do a manual rolling upgrade of all node roles, to a version including feature/efm-recovery.

will not work, if v1 nodes immediately begin produce v1 chunks with non-nil ServiceEventsNumber field.

This means the upgrade needs to be split into multiple steps, because v0 software will be unable to read v1 data models with non-nil new fields (will produce different hashes). Instead, we need:

1. Rolling upgrade from software version v0 to v1 (v1 still produces chunks with nil field)
2. Protocol HCU, after which v0 nodes cannot progress. At this point, v1 nodes begin producing v1 chunk models with non-nil fields. Chunk models with nil fields are also still accepted (due to sealing lag).
3. Protocol HCU, after which v0 data models are no longer accepted in new blocks

Because of the above additional complexity, I think we should revert the ProtocolStateVersionUpgrade service event to be emitted in the system chunk (at least to start), because we need Protocol HCUs to safely roll out the breaking chunk model change. After the upgrade is complete, all subsequent service events may be emitted outside the system chunk.

I think we can still do with just one HCU, but we might need 2 rolling upgrades. This is the steps, basically Step 1 and Step 3 are rolling upgrades, Step 2 is an protocol HCU:

1. Rolling upgrade from software version v0 to v1 (v1 still produces chunks with nil field)
   Making sure that:

• v1 EN will produce v1 result that have chunks with nil field, so that v1 result can be accepted by both v0 SN and v1 SN.
• v1 result with nil field in chunk produce the same result ID as the v0 result decoded from the v1 result.
• v1 block with the v1 result that has nil field, also produce the same block ID as the v0 block with the decoded v0 result, so that v1 block can be accepted by both v0 nodes or v1 nodes.

2. Protocol HCU, after which results with nil field will not be accepted. At this point, v1 ENs begin producing v1 chunk models with non-nil fields for blocks above HCU height. Details:

Once an protocol HCU event is emitted, then:

• When v1 SN receiving results, it will reject results with nil field below the HCU height.
• When v1 SN building blocks for height above the HCU height, it will include v1 results with non-nil fields only. Note, due to sealing lag, v1 SN might build some blocks that still have some results with nil field, because they are for blocks below the HCU height.
• When any v1 nodes receiving blocks, it will reject any block that contains result above the HCU height that has nil field in chunks
• Blocks produced from v0 SN for blocks above HCU won’t be accepted by v1 SN
• v0 SN cannot process after HCU because it won’t accept blocks from v1 SN (the block hash doesn’t match)

3. Rolling upgrade that v0 result or v1 result with nil field will not be accepted in new blocks

• Once HCU height in step 2 has been sealed, and all ENs are on v1, we can roll out this upgrade, because we are sure all results in new blocks won’t have nil field in chunks.

@zhangchiqing I agree that would work. Though, I suspect we will prefer fewer software upgrades, so we depend less on actions by node operators, but it's good to know that we have flexibility.

I have written a version of the upgrade plan based on our last discussion (with 2 HCUs and 1 rolling upgrade) here. The upgrade process is fairly simple - there is also an enumeration of versions, which I hope can be generalized beyond this specific example.